EmailDigestAgent relies on received events, rather in memory (#1624)

* EmailDigestAgent relies on received events, rather than storing them in memory

- Addressing a problem where 40,000+ events were stored in one agent's memory, and the agent never loaded from the database

* Cleans up references to memory["queue"]

- incorporates suggested style and safeguards

* Adds clarification on event inclusion in email

* Use `reorder` rather than stripping away all scoping

Will Read 8 年之前
父節點
當前提交
a6fe9743f6

+ 10 - 10
app/models/agents/email_digest_agent.rb

@@ -7,7 +7,9 @@ module Agents
7 7
     cannot_create_events!
8 8
 
9 9
     description <<-MD
10
-      The Email Digest Agent collects any Events sent to it and sends them all via email when scheduled.
10
+      The Email Digest Agent collects any Events sent to it and sends them all via email when scheduled. The number of
11
+      used events also relies on the `Keep events` option of the emitting Agent, meaning that if events expire before
12
+      this agent is scheduled to run, they will not appear in the email.
11 13
 
12 14
       By default, the will have a `subject` and an optional `headline` before listing the Events.  If the Events'
13 15
       payloads contain a `message`, that will be highlighted, otherwise everything in
@@ -37,18 +39,16 @@ module Agents
37 39
     end
38 40
 
39 41
     def receive(incoming_events)
42
+      self.memory['events'] ||= []
40 43
       incoming_events.each do |event|
41
-        self.memory['queue'] ||= []
42
-        self.memory['queue'] << event.payload
43
-        self.memory['events'] ||= []
44 44
         self.memory['events'] << event.id
45 45
       end
46 46
     end
47 47
 
48 48
     def check
49
-      if self.memory['queue'] && self.memory['queue'].length > 0
50
-        ids = self.memory['events'].join(",")
51
-        groups = self.memory['queue'].map { |payload| present(payload) }
49
+      if self.memory['events'] && self.memory['events'].length > 0
50
+        payloads = received_events.reorder("events.id ASC").where(id: self.memory['events']).pluck(:payload).to_a
51
+        groups = payloads.map { |payload| present(payload) }
52 52
         recipients.each do |recipient|
53 53
           begin
54 54
             SystemMailer.send_message(
@@ -59,13 +59,13 @@ module Agents
59 59
               content_type: interpolated['content_type'],
60 60
               groups: groups
61 61
             ).deliver_now
62
-            log "Sent digest mail to #{recipient} with events [#{ids}]"
62
+
63
+            log "Sent digest mail to #{recipient}"
63 64
           rescue => e
64
-            error("Error sending digest mail to #{recipient} with events [#{ids}]: #{e.message}")
65
+            error("Error sending digest mail to #{recipient}: #{e.message}")
65 66
             raise
66 67
           end
67 68
         end
68
-        self.memory['queue'] = []
69 69
         self.memory['events'] = []
70 70
       end
71 71
     end

+ 8 - 0
db/migrate/20160807000122_remove_queue_from_email_digest_agent_memory.rb

@@ -0,0 +1,8 @@
1
+class RemoveQueueFromEmailDigestAgentMemory < ActiveRecord::Migration
2
+  def up
3
+    Agents::EmailDigestAgent.find_each do |agent|
4
+      agent.memory.delete("queue")
5
+      agent.save!(validate: false)
6
+    end
7
+  end
8
+end

+ 28 - 25
spec/models/agents/email_digest_agent_spec.rb

@@ -8,11 +8,11 @@ describe Agents::EmailDigestAgent do
8 8
   end
9 9
 
10 10
   before do
11
-    @checker = Agents::EmailDigestAgent.new(:name => "something", :options => { :expected_receive_period_in_days => "2", :subject => "something interesting" })
11
+    @checker = Agents::EmailDigestAgent.new(:name => "something", :options => {:expected_receive_period_in_days => "2", :subject => "something interesting"})
12 12
     @checker.user = users(:bob)
13 13
     @checker.save!
14 14
 
15
-    @checker1 = Agents::EmailDigestAgent.new(:name => "something", :options => { :expected_receive_period_in_days => "2", :subject => "something interesting", :content_type => "text/plain" })
15
+    @checker1 = Agents::EmailDigestAgent.new(:name => "something", :options => {:expected_receive_period_in_days => "2", :subject => "something interesting", :content_type => "text/plain"})
16 16
     @checker1.user = users(:bob)
17 17
     @checker1.save!
18 18
   end
@@ -25,16 +25,16 @@ describe Agents::EmailDigestAgent do
25 25
     it "queues any payloads it receives" do
26 26
       event1 = Event.new
27 27
       event1.agent = agents(:bob_rain_notifier_agent)
28
-      event1.payload = { :data => "Something you should know about" }
28
+      event1.payload = {:data => "Something you should know about"}
29 29
       event1.save!
30 30
 
31 31
       event2 = Event.new
32 32
       event2.agent = agents(:bob_weather_agent)
33
-      event2.payload = { :data => "Something else you should know about" }
33
+      event2.payload = {:data => "Something else you should know about"}
34 34
       event2.save!
35 35
 
36 36
       Agents::EmailDigestAgent.async_receive(@checker.id, [event1.id, event2.id])
37
-      expect(@checker.reload.memory[:queue]).to eq([{ 'data' => "Something you should know about" }, { 'data' => "Something else you should know about" }])
37
+      expect(@checker.reload.memory['events']).to match([event1.id, event2.id])
38 38
     end
39 39
   end
40 40
 
@@ -44,25 +44,34 @@ describe Agents::EmailDigestAgent do
44 44
       Agents::EmailDigestAgent.async_check(@checker.id)
45 45
       expect(ActionMailer::Base.deliveries).to eq([])
46 46
 
47
-      @checker.memory[:queue] = [{ :data => "Something you should know about" },
48
-                                 { :title => "Foo", :url => "http://google.com", :bar => 2 },
49
-                                 { "message" => "hi", :woah => "there" },
50
-                                 { "test" => 2 }]
51
-      @checker.memory[:events] = [1,2,3,4]
52
-      @checker.save!
53
-
54
-      Agents::EmailDigestAgent.async_check(@checker.id)
47
+      payloads = [
48
+        {:data => "Something you should know about"},
49
+        {:title => "Foo", :url => "http://google.com", :bar => 2},
50
+        {"message" => "hi", :woah => "there"},
51
+        {"test" => 2}
52
+      ]
53
+
54
+      events = payloads.map do |payload|
55
+        Event.new.tap do |event|
56
+          event.agent = agents(:bob_weather_agent)
57
+          event.payload = payload
58
+          event.save!
59
+        end
60
+      end
61
+
62
+      Agents::DigestAgent.async_receive(@checker.id, events.map(&:id))
63
+      @checker.sources << agents(:bob_weather_agent)
64
+      Agents::DigestAgent.async_check(@checker.id)
55 65
 
56 66
       expect(ActionMailer::Base.deliveries.last.to).to eq(["bob@example.com"])
57 67
       expect(ActionMailer::Base.deliveries.last.subject).to eq("something interesting")
58 68
       expect(get_message_part(ActionMailer::Base.deliveries.last, /plain/).strip).to eq("Event\n  data: Something you should know about\n\nFoo\n  bar: 2\n  url: http://google.com\n\nhi\n  woah: there\n\nEvent\n  test: 2")
59
-      expect(@checker.reload.memory[:queue]).to be_empty
69
+      expect(@checker.reload.memory[:events]).to be_empty
60 70
     end
61 71
 
62 72
     it "logs and re-raises mailer errors" do
63 73
       mock(SystemMailer).send_message(anything) { raise Net::SMTPAuthenticationError.new("Wrong password") }
64 74
 
65
-      @checker.memory[:queue] = [{ :data => "Something you should know about" }]
66 75
       @checker.memory[:events] = [1]
67 76
       @checker.save!
68 77
 
@@ -71,8 +80,6 @@ describe Agents::EmailDigestAgent do
71 80
       }.to raise_error(/Wrong password/)
72 81
 
73 82
       expect(@checker.reload.memory[:events]).not_to be_empty
74
-      expect(@checker.reload.memory[:queue]).not_to be_empty
75
-
76 83
       expect(@checker.logs.last.message).to match(/Error sending digest mail .* Wrong password/)
77 84
     end
78 85
 
@@ -84,7 +91,7 @@ describe Agents::EmailDigestAgent do
84 91
       Agent.async_check(agents(:bob_weather_agent).id)
85 92
 
86 93
       Agent.receive!
87
-      expect(@checker.reload.memory[:queue]).not_to be_empty
94
+      expect(@checker.reload.memory[:events]).not_to be_empty
88 95
 
89 96
       Agents::EmailDigestAgent.async_check(@checker.id)
90 97
 
@@ -94,18 +101,14 @@ describe Agents::EmailDigestAgent do
94 101
       expect(plain_email_text).to match(/avehumidity/)
95 102
       expect(html_email_text).to match(/avehumidity/)
96 103
 
97
-      expect(@checker.reload.memory[:queue]).to be_empty
104
+      expect(@checker.reload.memory[:events]).to be_empty
98 105
     end
99
-    
106
+
100 107
     it "should send email with correct content type" do
101 108
       Agents::EmailDigestAgent.async_check(@checker1.id)
102 109
       expect(ActionMailer::Base.deliveries).to eq([])
103 110
 
104
-      @checker1.memory[:queue] = [{ :data => "Something you should know about" },
105
-                                 { :title => "Foo", :url => "http://google.com", :bar => 2 },
106
-                                 { "message" => "hi", :woah => "there" },
107
-                                 { "test" => 2 }]
108
-      @checker1.memory[:events] = [1,2,3,4]
111
+      @checker1.memory[:events] = [1, 2, 3, 4]
109 112
       @checker1.save!
110 113
 
111 114
       Agents::EmailDigestAgent.async_check(@checker1.id)